[Qemu-devel] [PATCH v3 0/9] arm: Steps towards EL2 support round 5

[Qemu-devel] [PATCH v3 0/9] arm: Steps towards EL2 support round 5

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

Another round of patches towards EL2 support. This one adds partial
support for 2-stage MMU. The AArch32/ARMv7 support is untested.

Some of the details of error reporting are intentionally missing, I
was thinking to add those incrementally as they get quite involved
(e.g the register target and memory access size).

Comments welcome!

Best regards,
Edgar

v2 -> v3:
* Prettify comments for ARMMMUFaultInfo
* Add S2 translation for 32bit S1 PTWs
* Add more comments to S2 PTW starting level computation.

v1 -> v2:
* Fix HPFAR_EL2 access checks
* Prettify computation of starting level for S2 PTW
* Improve description of ap argument to get_S2prot
* Fix EXEC protection in get_S2prot
* Improve comments on S2 PTW attribute extraction
* Add comment describing ARMMMUFaultInfo

Edgar E. Iglesias (9):
  target-arm: Add HPFAR_EL2
  target-arm: Add computation of starting level for S2 PTW
  target-arm: Add support for S2 page-table protection bits
  target-arm: Avoid inline for get_phys_addr
  target-arm: Add ARMMMUFaultInfo
  target-arm: Add S2 translation to 64bit S1 PTWs
  target-arm: Add S2 translation to 32bit S1 PTWs
  target-arm: Route S2 MMU faults to EL2
  target-arm: Add support for S1 + S2 MMU translations

 target-arm/cpu.h       |   1 +
 target-arm/helper.c    | 252 +++++++++++++++++++++++++++++++++++++++----------
 target-arm/internals.h |  15 ++-
 target-arm/op_helper.c |  17 +++-
 4 files changed, 231 insertions(+), 54 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/cpu.h    |  1 +
 target-arm/helper.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cc1578c..895f2c2 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -278,6 +278,7 @@ typedef struct CPUARMState {
             };
             uint64_t far_el[4];
         };
+        uint64_t hpfar_el2;
         union { /* Translation result. */
             struct {
                 uint64_t _unused_par_0;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8367997..5a5e5f0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
     { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
+      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     REGINFO_SENTINEL
 };
 
@@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .resetvalue = 0,
       .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
 #endif
+    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
+      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
+      .access = PL2_RW, .accessfn = access_el3_aa32ns,
+      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
+    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
+      .access = PL2_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
     REGINFO_SENTINEL
 };
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Alex Bennée]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cc1578c..895f2c2 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>              };
>              uint64_t far_el[4];
>          };
> +        uint64_t hpfar_el2;
>          union { /* Translation result. */
>              struct {
>                  uint64_t _unused_par_0;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8367997..5a5e5f0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },

So what happens if access_el3_aa32ns_aa64any thinks it is OK to access
the register from EL3 when there is no EL2? What ensures we get RES0?

>      REGINFO_SENTINEL
>  };
>  
> @@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .resetvalue = 0,
>        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
>  #endif
> +    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW, .accessfn = access_el3_aa32ns,
> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>      REGINFO_SENTINEL
>  };

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Peter Maydell]

On 7 October 2015 at 12:51, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> ---
>>  target-arm/cpu.h    |  1 +
>>  target-arm/helper.c | 12 ++++++++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index cc1578c..895f2c2 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>>              };
>>              uint64_t far_el[4];
>>          };
>> +        uint64_t hpfar_el2;
>>          union { /* Translation result. */
>>              struct {
>>                  uint64_t _unused_par_0;
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8367997..5a5e5f0 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>
> So what happens if access_el3_aa32ns_aa64any thinks it is OK to access
> the register from EL3 when there is no EL2? What ensures we get RES0?

...the fact we've defined it as an RW CONST register with a resetvalue
of zero? Or am I misunderstanding your question?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 October 2015 at 12:51, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>>
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> ---
>>>  target-arm/cpu.h    |  1 +
>>>  target-arm/helper.c | 12 ++++++++++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index cc1578c..895f2c2 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>>>              };
>>>              uint64_t far_el[4];
>>>          };
>>> +        uint64_t hpfar_el2;
>>>          union { /* Translation result. */
>>>              struct {
>>>                  uint64_t _unused_par_0;
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 8367997..5a5e5f0 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>>>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>>
>> So what happens if access_el3_aa32ns_aa64any thinks it is OK to access
>> the register from EL3 when there is no EL2? What ensures we get RES0?
>
> ...the fact we've defined it as an RW CONST register with a resetvalue
> of zero? Or am I misunderstanding your question?

Nope you didn't misunderstand, I was being dim comparing with the later
definitions ;-)

Thanks.

>
> thanks
> -- PMM

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Laurent Desnogues]

Hello,

On Sun, Oct 4, 2015 at 12:38 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cc1578c..895f2c2 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>              };
>              uint64_t far_el[4];
>          };
> +        uint64_t hpfar_el2;
>          union { /* Translation result. */
>              struct {
>                  uint64_t _unused_par_0;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8367997..5a5e5f0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      REGINFO_SENTINEL
>  };
>
> @@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .resetvalue = 0,
>        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
>  #endif
> +    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW, .accessfn = access_el3_aa32ns,
> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>      REGINFO_SENTINEL
>  };

Shouldn't these last two registers be placed before the "#endif" which
closes an "#ifndef CONFIG_USER_ONLY"?

Thanks,

Laurent

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Peter Maydell]

On 8 October 2015 at 06:38, Laurent Desnogues
<laurent.desnogues@gmail.com> wrote:
> Hello,
>
> On Sun, Oct 4, 2015 at 12:38 AM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> ---
>>  target-arm/cpu.h    |  1 +
>>  target-arm/helper.c | 12 ++++++++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index cc1578c..895f2c2 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>>              };
>>              uint64_t far_el[4];
>>          };
>> +        uint64_t hpfar_el2;
>>          union { /* Translation result. */
>>              struct {
>>                  uint64_t _unused_par_0;
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8367997..5a5e5f0 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>>      REGINFO_SENTINEL
>>  };
>>
>> @@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>>        .resetvalue = 0,
>>        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
>>  #endif
>> +    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
>> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>> +      .access = PL2_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>>      REGINFO_SENTINEL
>>  };
>
> Shouldn't these last two registers be placed before the "#endif" which
> closes an "#ifndef CONFIG_USER_ONLY"?

Mostly I've taken the approach that we only ifdef out regdefs
which actually won't compile (the timer ones refer to functions
which call softmmu-only timer related functions, for instance).
Having regdefs for higher ELs in the USER_ONLY build is harmless;
they just sit in the hash table and are never accessed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Alex Bennée]

Laurent Desnogues <laurent.desnogues@gmail.com> writes:

> Hello,
>
> On Sun, Oct 4, 2015 at 12:38 AM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> ---
>>  target-arm/cpu.h    |  1 +
>>  target-arm/helper.c | 12 ++++++++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index cc1578c..895f2c2 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>>              };
>>              uint64_t far_el[4];
>>          };
>> +        uint64_t hpfar_el2;
>>          union { /* Translation result. */
>>              struct {
>>                  uint64_t _unused_par_0;
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8367997..5a5e5f0 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>>      REGINFO_SENTINEL
>>  };
>>
>> @@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>>        .resetvalue = 0,
>>        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
>>  #endif
>> +    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
>> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>> +      .access = PL2_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>>      REGINFO_SENTINEL
>>  };
>
> Shouldn't these last two registers be placed before the "#endif" which
> closes an "#ifndef CONFIG_USER_ONLY"?

There seem to be a bunch of _EL2 registers above the #ifndef as well so
I'm not sure it matters. In fact won't the guest just trap if it tries?

>
> Thanks,
>
> Laurent

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Laurent Desnogues]

On Thu, Oct 8, 2015 at 10:24 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Laurent Desnogues <laurent.desnogues@gmail.com> writes:
>
>> Hello,
>>
>> On Sun, Oct 4, 2015 at 12:38 AM, Edgar E. Iglesias
>> <edgar.iglesias@gmail.com> wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> ---
>>>  target-arm/cpu.h    |  1 +
>>>  target-arm/helper.c | 12 ++++++++++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index cc1578c..895f2c2 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>>>              };
>>>              uint64_t far_el[4];
>>>          };
>>> +        uint64_t hpfar_el2;
>>>          union { /* Translation result. */
>>>              struct {
>>>                  uint64_t _unused_par_0;
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 8367997..5a5e5f0 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>>>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>>>      REGINFO_SENTINEL
>>>  };
>>>
>>> @@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>>>        .resetvalue = 0,
>>>        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
>>>  #endif
>>> +    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
>>> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>>> +      .access = PL2_RW, .accessfn = access_el3_aa32ns,
>>> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>>> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
>>> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>>> +      .access = PL2_RW,
>>> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>>>      REGINFO_SENTINEL
>>>  };
>>
>> Shouldn't these last two registers be placed before the "#endif" which
>> closes an "#ifndef CONFIG_USER_ONLY"?
>
> There seem to be a bunch of _EL2 registers above the #ifndef as well so
> I'm not sure it matters. In fact won't the guest just trap if it tries?

That's correct.

But over the years a lot of code has been added that is completely
irrelevant to user mode.  An interesting experiment is to check how
the interrupt_request field of CPU is in fact not needed by
arm-linux-user.  You can prove it's only needed through a chain of
calls that originate from omap_wfi_write which is PL1_RW.  I agree
it's not a big deal, it's most likely only me who finds it ugly and
inefficient that an interrupt request field exists in user mode :-)

Anyway this in no case is a reason to not ack Edgar patch.


Laurent

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Alex Bennée]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Now Peter has pointed out I can't read ;-)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cc1578c..895f2c2 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -278,6 +278,7 @@ typedef struct CPUARMState {
>              };
>              uint64_t far_el[4];
>          };
> +        uint64_t hpfar_el2;
>          union { /* Translation result. */
>              struct {
>                  uint64_t _unused_par_0;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8367997..5a5e5f0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      REGINFO_SENTINEL
>  };
>  
> @@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .resetvalue = 0,
>        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
>  #endif
> +    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW, .accessfn = access_el3_aa32ns,
> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
> +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> +      .access = PL2_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
>      REGINFO_SENTINEL
>  };

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2

[Edgar E. Iglesias]

On Thu, Oct 08, 2015 at 10:14:08AM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> Now Peter has pointed out I can't read ;-)
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


Thanks for all the clarifications, I'll add your RB to my series.

Best regards,
Edgar




> 
> > ---
> >  target-arm/cpu.h    |  1 +
> >  target-arm/helper.c | 12 ++++++++++++
> >  2 files changed, 13 insertions(+)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index cc1578c..895f2c2 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -278,6 +278,7 @@ typedef struct CPUARMState {
> >              };
> >              uint64_t far_el[4];
> >          };
> > +        uint64_t hpfar_el2;
> >          union { /* Translation result. */
> >              struct {
> >                  uint64_t _unused_par_0;
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 8367997..5a5e5f0 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3223,6 +3223,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
> >      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
> >        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
> >        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> > +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
> > +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> > +      .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
> > +      .type = ARM_CP_CONST, .resetvalue = 0 },
> >      REGINFO_SENTINEL
> >  };
> >  
> > @@ -3444,6 +3448,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
> >        .resetvalue = 0,
> >        .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
> >  #endif
> > +    { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
> > +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> > +      .access = PL2_RW, .accessfn = access_el3_aa32ns,
> > +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
> > +    { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64,
> > +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
> > +      .access = PL2_RW,
> > +      .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) },
> >      REGINFO_SENTINEL
> >  };
> 
> -- 
> Alex Bennée

[Qemu-devel] [PATCH v3 2/9] target-arm: Add computation of starting level for S2 PTW

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

The starting level for S2 pagetable walks is computed
differently from the S1 starting level. Implement the S2
variant.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 5a5e5f0..507324f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6549,18 +6549,33 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         goto do_fault;
     }
 
-    /* The starting level depends on the virtual address size (which can be
-     * up to 48 bits) and the translation granule size. It indicates the number
-     * of strides (granule_sz bits at a time) needed to consume the bits
-     * of the input address. In the pseudocode this is:
-     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
-     * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
-     * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
-     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
-     *     = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
-     *     = 4 - (va_size - tsz - 4) / granule_sz;
-     */
-    level = 4 - (va_size - tsz - 4) / granule_sz;
+    if (mmu_idx != ARMMMUIdx_S2NS) {
+        /* The starting level depends on the virtual address size (which can
+         * be up to 48 bits) and the translation granule size. It indicates
+         * the number of strides (granule_sz bits at a time) needed to
+         * consume the bits of the input address. In the pseudocode this is:
+         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
+         * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
+         * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
+         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
+         * = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
+         * = 4 - (va_size - tsz - 4) / granule_sz;
+         */
+        level = 4 - (va_size - tsz - 4) / granule_sz;
+    } else {
+        unsigned int startlevel = extract32(tcr->raw_tcr, 6, 2);
+
+        /* For stage 2 translations the starting level is specified by the
+         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)
+         */
+        if (granule_sz == 9) {
+            /* 4K pages */
+            level = 2 - startlevel;
+        } else {
+            /* 16K or 64K pages */
+            level = 3 - startlevel;
+        }
+    }
 
     /* Clear the vaddr bits which aren't part of the within-region address,
      * so that we don't have to special case things when calculating the
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 2/9] target-arm: Add computation of starting level for S2 PTW

[Alex Bennée]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> The starting level for S2 pagetable walks is computed
> differently from the S1 starting level. Implement the S2
> variant.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 5a5e5f0..507324f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6549,18 +6549,33 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          goto do_fault;
>      }
>  
> -    /* The starting level depends on the virtual address size (which can be
> -     * up to 48 bits) and the translation granule size. It indicates the number
> -     * of strides (granule_sz bits at a time) needed to consume the bits
> -     * of the input address. In the pseudocode this is:
> -     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> -     * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
> -     * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
> -     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> -     *     = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
> -     *     = 4 - (va_size - tsz - 4) / granule_sz;
> -     */
> -    level = 4 - (va_size - tsz - 4) / granule_sz;
> +    if (mmu_idx != ARMMMUIdx_S2NS) {
> +        /* The starting level depends on the virtual address size (which can
> +         * be up to 48 bits) and the translation granule size. It indicates
> +         * the number of strides (granule_sz bits at a time) needed to
> +         * consume the bits of the input address. In the pseudocode this is:
> +         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> +         * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
> +         * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
> +         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> +         * = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
> +         * = 4 - (va_size - tsz - 4) / granule_sz;
> +         */
> +        level = 4 - (va_size - tsz - 4) / granule_sz;
> +    } else {
> +        unsigned int startlevel = extract32(tcr->raw_tcr, 6, 2);

Maybe an assert(startlevel<3) would be useful?

> +
> +        /* For stage 2 translations the starting level is specified by the
> +         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)
> +         */
> +        if (granule_sz == 9) {
> +            /* 4K pages */
> +            level = 2 - startlevel;
> +        } else {
> +            /* 16K or 64K pages */
> +            level = 3 - startlevel;
> +        }
> +    }
>  
>      /* Clear the vaddr bits which aren't part of the within-region address,
>       * so that we don't have to special case things when calculating the

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 2/9] target-arm: Add computation of starting level for S2 PTW

[Edgar E. Iglesias]

On Wed, Oct 07, 2015 at 01:24:27PM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > The starting level for S2 pagetable walks is computed
> > differently from the S1 starting level. Implement the S2
> > variant.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c | 39 +++++++++++++++++++++++++++------------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 5a5e5f0..507324f 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6549,18 +6549,33 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          goto do_fault;
> >      }
> >  
> > -    /* The starting level depends on the virtual address size (which can be
> > -     * up to 48 bits) and the translation granule size. It indicates the number
> > -     * of strides (granule_sz bits at a time) needed to consume the bits
> > -     * of the input address. In the pseudocode this is:
> > -     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> > -     * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
> > -     * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
> > -     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> > -     *     = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
> > -     *     = 4 - (va_size - tsz - 4) / granule_sz;
> > -     */
> > -    level = 4 - (va_size - tsz - 4) / granule_sz;
> > +    if (mmu_idx != ARMMMUIdx_S2NS) {
> > +        /* The starting level depends on the virtual address size (which can
> > +         * be up to 48 bits) and the translation granule size. It indicates
> > +         * the number of strides (granule_sz bits at a time) needed to
> > +         * consume the bits of the input address. In the pseudocode this is:
> > +         *  level = 4 - RoundUp((inputsize - grainsize) / stride)
> > +         * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
> > +         * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
> > +         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> > +         * = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
> > +         * = 4 - (va_size - tsz - 4) / granule_sz;
> > +         */
> > +        level = 4 - (va_size - tsz - 4) / granule_sz;
> > +    } else {
> > +        unsigned int startlevel = extract32(tcr->raw_tcr, 6, 2);
> 
> Maybe an assert(startlevel<3) would be useful?

Good catch here. There is actually a trap for some of the bad startlevel cases.
I'll implement that for the next version.


> 
> > +
> > +        /* For stage 2 translations the starting level is specified by the
> > +         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)
> > +         */
> > +        if (granule_sz == 9) {
> > +            /* 4K pages */
> > +            level = 2 - startlevel;
> > +        } else {
> > +            /* 16K or 64K pages */
> > +            level = 3 - startlevel;
> > +        }
> > +    }
> >  
> >      /* Clear the vaddr bits which aren't part of the within-region address,
> >       * so that we don't have to special case things when calculating the
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks, I'll hold the RB until you see the next version with the traps for bad
start-levels.

Cheers,
Edgar

[Qemu-devel] [PATCH v3 3/9] target-arm: Add support for S2 page-table protection bits

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 507324f..610f1b5 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6015,6 +6015,28 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
     return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
 }
 
+/* Translate S2 section/page access permissions to protection flags
+ *
+ * @env:     CPUARMState
+ * @s2ap:    The 2-bit stage2 access permissions (S2AP)
+ * @xn:      XN (execute-never) bit
+ */
+static int get_S2prot(CPUARMState *env, int s2ap, int xn)
+{
+    int prot = 0;
+
+    if (s2ap & 1) {
+        prot |= PAGE_READ;
+    }
+    if (s2ap & 2) {
+        prot |= PAGE_WRITE;
+    }
+    if (!xn) {
+        prot |= PAGE_EXEC;
+    }
+    return prot;
+}
+
 /* Translate section/page access permissions to protection flags
  *
  * @env:     CPUARMState
@@ -6628,9 +6650,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
          */
         page_size = (1ULL << ((granule_sz * (4 - level)) + 3));
         descaddr |= (address & (page_size - 1));
-        /* Extract attributes from the descriptor and merge with table attrs */
+        /* Extract attributes from the descriptor */
         attrs = extract64(descriptor, 2, 10)
             | (extract64(descriptor, 52, 12) << 10);
+
+        if (mmu_idx == ARMMMUIdx_S2NS) {
+            /* Stage 2 table descriptors do not include any attribute fields */
+            break;
+        }
+        /* Merge in attributes from table descriptors */
         attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
         attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
         /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
@@ -6652,11 +6680,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     }
 
     ap = extract32(attrs, 4, 2);
-    ns = extract32(attrs, 3, 1);
     xn = extract32(attrs, 12, 1);
-    pxn = extract32(attrs, 11, 1);
 
-    *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        ns = true;
+        *prot = get_S2prot(env, ap, xn);
+    } else {
+        ns = extract32(attrs, 3, 1);
+        pxn = extract32(attrs, 11, 1);
+        *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
+    }
 
     fault_type = permission_fault;
     if (!(*prot & (1 << access_type))) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 3/9] target-arm: Add support for S2 page-table protection bits

[Alex Bennée]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 507324f..610f1b5 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6015,6 +6015,28 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
>      return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
>  }
>  
> +/* Translate S2 section/page access permissions to protection flags
> + *
> + * @env:     CPUARMState
> + * @s2ap:    The 2-bit stage2 access permissions (S2AP)
> + * @xn:      XN (execute-never) bit
> + */
> +static int get_S2prot(CPUARMState *env, int s2ap, int xn)
> +{
> +    int prot = 0;
> +
> +    if (s2ap & 1) {
> +        prot |= PAGE_READ;
> +    }
> +    if (s2ap & 2) {
> +        prot |= PAGE_WRITE;
> +    }
> +    if (!xn) {
> +        prot |= PAGE_EXEC;
> +    }
> +    return prot;
> +}
> +
>  /* Translate section/page access permissions to protection flags
>   *
>   * @env:     CPUARMState
> @@ -6628,9 +6650,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>           */
>          page_size = (1ULL << ((granule_sz * (4 - level)) + 3));
>          descaddr |= (address & (page_size - 1));
> -        /* Extract attributes from the descriptor and merge with table attrs */
> +        /* Extract attributes from the descriptor */
>          attrs = extract64(descriptor, 2, 10)
>              | (extract64(descriptor, 52, 12) << 10);
> +
> +        if (mmu_idx == ARMMMUIdx_S2NS) {
> +            /* Stage 2 table descriptors do not include any attribute fields */
> +            break;
> +        }
> +        /* Merge in attributes from table descriptors */
>          attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
>          attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
>          /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
> @@ -6652,11 +6680,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      }
>  
>      ap = extract32(attrs, 4, 2);
> -    ns = extract32(attrs, 3, 1);
>      xn = extract32(attrs, 12, 1);
> -    pxn = extract32(attrs, 11, 1);

OK I've gotten lost in the ARM ARM. Is there an architecture defined
format the final attrs we construct from the page tables is meant to
conform to? Or is the choice of the final structure arbitrary?

>  
> -    *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        ns = true;
> +        *prot = get_S2prot(env, ap, xn);
> +    } else {
> +        ns = extract32(attrs, 3, 1);
> +        pxn = extract32(attrs, 11, 1);
> +        *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> +    }
>  
>      fault_type = permission_fault;
>      if (!(*prot & (1 << access_type))) {

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 3/9] target-arm: Add support for S2 page-table protection bits

[Peter Maydell]

On 7 October 2015 at 17:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> ---
>>  target-arm/helper.c | 41 +++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 507324f..610f1b5 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -6015,6 +6015,28 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
>>      return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
>>  }
>>
>> +/* Translate S2 section/page access permissions to protection flags
>> + *
>> + * @env:     CPUARMState
>> + * @s2ap:    The 2-bit stage2 access permissions (S2AP)
>> + * @xn:      XN (execute-never) bit
>> + */
>> +static int get_S2prot(CPUARMState *env, int s2ap, int xn)
>> +{
>> +    int prot = 0;
>> +
>> +    if (s2ap & 1) {
>> +        prot |= PAGE_READ;
>> +    }
>> +    if (s2ap & 2) {
>> +        prot |= PAGE_WRITE;
>> +    }
>> +    if (!xn) {
>> +        prot |= PAGE_EXEC;
>> +    }
>> +    return prot;
>> +}
>> +
>>  /* Translate section/page access permissions to protection flags
>>   *
>>   * @env:     CPUARMState
>> @@ -6628,9 +6650,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>           */
>>          page_size = (1ULL << ((granule_sz * (4 - level)) + 3));
>>          descaddr |= (address & (page_size - 1));
>> -        /* Extract attributes from the descriptor and merge with table attrs */
>> +        /* Extract attributes from the descriptor */
>>          attrs = extract64(descriptor, 2, 10)
>>              | (extract64(descriptor, 52, 12) << 10);
>> +
>> +        if (mmu_idx == ARMMMUIdx_S2NS) {
>> +            /* Stage 2 table descriptors do not include any attribute fields */
>> +            break;
>> +        }
>> +        /* Merge in attributes from table descriptors */
>>          attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
>>          attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
>>          /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
>> @@ -6652,11 +6680,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>      }
>>
>>      ap = extract32(attrs, 4, 2);
>> -    ns = extract32(attrs, 3, 1);
>>      xn = extract32(attrs, 12, 1);
>> -    pxn = extract32(attrs, 11, 1);
>
> OK I've gotten lost in the ARM ARM. Is there an architecture defined
> format the final attrs we construct from the page tables is meant to
> conform to? Or is the choice of the final structure arbitrary?

The bit of the ARM ARM you want is D4.3.3 in rev A.g of the v8 ARM ARM,
which describes all the attribute fields in the various descriptors.

At this point 'attrs' is in the format of the attribute bits from
a stage 1 Block/Page descriptor, all shifted down to the bottom of
a word (ie attrs[21:0] is descriptor bits [63:52] + [11:2]). You can
see us shifting [63:52] and [11:2] into place in the line just below
the "Extract attributes from the descriptor" line. tableattrs[4:0] is
bits [63:59] of the stage 1 Table descriptor format.

For stage 2 translations (which is what the code Edgar's adding
is handling) the "stage 2 block and page descriptors" format is
a bit different (mostly it's missing bits that don't have any
meaning for stage 2), and stage 2 table descriptors have no
attribute bits at all.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/9] target-arm: Add support for S2 page-table protection bits

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 October 2015 at 17:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>>
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> ---
>>>  target-arm/helper.c | 41 +++++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 37 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 507324f..610f1b5 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -6015,6 +6015,28 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
>>>      return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
>>>  }
>>>
>>> +/* Translate S2 section/page access permissions to protection flags
>>> + *
>>> + * @env:     CPUARMState
>>> + * @s2ap:    The 2-bit stage2 access permissions (S2AP)
>>> + * @xn:      XN (execute-never) bit
>>> + */
>>> +static int get_S2prot(CPUARMState *env, int s2ap, int xn)
>>> +{
>>> +    int prot = 0;
>>> +
>>> +    if (s2ap & 1) {
>>> +        prot |= PAGE_READ;
>>> +    }
>>> +    if (s2ap & 2) {
>>> +        prot |= PAGE_WRITE;
>>> +    }
>>> +    if (!xn) {
>>> +        prot |= PAGE_EXEC;
>>> +    }
>>> +    return prot;
>>> +}
>>> +
>>>  /* Translate section/page access permissions to protection flags
>>>   *
>>>   * @env:     CPUARMState
>>> @@ -6628,9 +6650,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>>           */
>>>          page_size = (1ULL << ((granule_sz * (4 - level)) + 3));
>>>          descaddr |= (address & (page_size - 1));
>>> -        /* Extract attributes from the descriptor and merge with table attrs */
>>> +        /* Extract attributes from the descriptor */
>>>          attrs = extract64(descriptor, 2, 10)
>>>              | (extract64(descriptor, 52, 12) << 10);
>>> +
>>> +        if (mmu_idx == ARMMMUIdx_S2NS) {
>>> +            /* Stage 2 table descriptors do not include any attribute fields */
>>> +            break;
>>> +        }
>>> +        /* Merge in attributes from table descriptors */
>>>          attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
>>>          attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
>>>          /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
>>> @@ -6652,11 +6680,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>>      }
>>>
>>>      ap = extract32(attrs, 4, 2);
>>> -    ns = extract32(attrs, 3, 1);
>>>      xn = extract32(attrs, 12, 1);
>>> -    pxn = extract32(attrs, 11, 1);
>>
>> OK I've gotten lost in the ARM ARM. Is there an architecture defined
>> format the final attrs we construct from the page tables is meant to
>> conform to? Or is the choice of the final structure arbitrary?
>
> The bit of the ARM ARM you want is D4.3.3 in rev A.g of the v8 ARM ARM,
> which describes all the attribute fields in the various descriptors.
>
> At this point 'attrs' is in the format of the attribute bits from
> a stage 1 Block/Page descriptor, all shifted down to the bottom of
> a word (ie attrs[21:0] is descriptor bits [63:52] + [11:2]). You can
> see us shifting [63:52] and [11:2] into place in the line just below
> the "Extract attributes from the descriptor" line. tableattrs[4:0] is
> bits [63:59] of the stage 1 Table descriptor format.

Right, so I was getting lost between these. Perhaps a handy diagram is
warranted?

        /* Extract attributes from the descriptors. We combine the
         * two sections [58:52] and [11:2] into a contiguous attrs
         * bitfield:
         *
         *O:58   55   54     53   52 / 11   10  9  8 7   6  5   4    2
         *  16   13   12     11   10    9    8  7  6 5   4  3   2    0
         * +-------+-------+-----+---+----+----+----+-----+----+------+
         * | SWUse | (U)XN | PXN | C | nG | AF | SH | AP* | NS | Attr |
         * +-------+-------+-----+---+----+----+----+-----+----+------+
         *
         * AP*, is S2AP for stage 2 translation
         */

While looking I noticed we probably extract more than we need into attrs
considering the tableattrs is snarfed earlier. Maybe we should only do:

        attrs = extract64(descriptor, 2, 10)
            | (extract64(descriptor, 52, 7) << 10);

Or even:

        attrs = extract64(descriptor, 2, 10)
            | (extract64(descriptor, 52, 3) << 10);

> For stage 2 translations (which is what the code Edgar's adding
> is handling) the "stage 2 block and page descriptors" format is
> a bit different (mostly it's missing bits that don't have any
> meaning for stage 2), and stage 2 table descriptors have no
> attribute bits at all.

Hmm yeah my ascii diagram kinda glosses over that :-/

Anyway I can follow what's going on now. I'm just left with a nagging
feeling that the code could be cleaner but I'm not sure how....

>
> thanks
> -- PMM

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 3/9] target-arm: Add support for S2 page-table protection bits

[Peter Maydell]

On 14 October 2015 at 13:45, Alex Bennée <alex.bennee@linaro.org> wrote:
> While looking I noticed we probably extract more than we need into attrs
> considering the tableattrs is snarfed earlier. Maybe we should only do:
>
>         attrs = extract64(descriptor, 2, 10)
>             | (extract64(descriptor, 52, 7) << 10);
>
> Or even:
>
>         attrs = extract64(descriptor, 2, 10)
>             | (extract64(descriptor, 52, 3) << 10);

It seemed neater to grab all of the attribute bits, even the
reserved ones. Otherwise we'll end up needing to change the
code back later if a new attribute we care about is added.

thanks
-- PMM

[Qemu-devel] [PATCH v3 4/9] target-arm: Avoid inline for get_phys_addr

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Avoid inline for get_phys_addr() to prepare for future recursive use.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 610f1b5..cbc1570 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -15,10 +15,10 @@
 #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
 
 #ifndef CONFIG_USER_ONLY
-static inline bool get_phys_addr(CPUARMState *env, target_ulong address,
-                                 int access_type, ARMMMUIdx mmu_idx,
-                                 hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                                 target_ulong *page_size, uint32_t *fsr);
+static bool get_phys_addr(CPUARMState *env, target_ulong address,
+                          int access_type, ARMMMUIdx mmu_idx,
+                          hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
+                          target_ulong *page_size, uint32_t *fsr);
 
 /* Definitions for the PMCCNTR and PMCR registers */
 #define PMCRD   0x8
@@ -6972,10 +6972,10 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
  * @page_size: set to the size of the page containing phys_ptr
  * @fsr: set to the DFSR/IFSR value on failure
  */
-static inline bool get_phys_addr(CPUARMState *env, target_ulong address,
-                                 int access_type, ARMMMUIdx mmu_idx,
-                                 hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                                 target_ulong *page_size, uint32_t *fsr)
+static bool get_phys_addr(CPUARMState *env, target_ulong address,
+                          int access_type, ARMMMUIdx mmu_idx,
+                          hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
+                          target_ulong *page_size, uint32_t *fsr)
 {
     if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
         /* TODO: when we support EL2 we should here call ourselves recursively
-- 
1.9.1

[Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Introduce ARMMMUFaultInfo to propagate MMU Fault information
across the MMU translation code path. This is in preparation for
adding State-2 translation.

No functional changes.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c    | 32 ++++++++++++++++++++------------
 target-arm/internals.h | 15 ++++++++++++++-
 target-arm/op_helper.c |  3 ++-
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index cbc1570..a429ff2 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -18,7 +18,8 @@
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           int access_type, ARMMMUIdx mmu_idx,
                           hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                          target_ulong *page_size, uint32_t *fsr);
+                          target_ulong *page_size, uint32_t *fsr,
+                          ARMMMUFaultInfo *fi);
 
 /* Definitions for the PMCCNTR and PMCR registers */
 #define PMCRD   0x8
@@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     bool ret;
     uint64_t par64;
     MemTxAttrs attrs = {};
+    ARMMMUFaultInfo fi = {};
 
     ret = get_phys_addr(env, value, access_type, mmu_idx,
-                        &phys_addr, &attrs, &prot, &page_size, &fsr);
+                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
     if (extended_addresses_enabled(env)) {
         /* fsr is a DFSR/IFSR value for the long descriptor
          * translation table format, but with WnR always clear.
@@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
 static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
                              int access_type, ARMMMUIdx mmu_idx,
                              hwaddr *phys_ptr, int *prot,
-                             target_ulong *page_size, uint32_t *fsr)
+                             target_ulong *page_size, uint32_t *fsr,
+                             ARMMMUFaultInfo *fi)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
     int code;
@@ -6280,7 +6283,8 @@ do_fault:
 static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
                              int access_type, ARMMMUIdx mmu_idx,
                              hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                             target_ulong *page_size, uint32_t *fsr)
+                             target_ulong *page_size, uint32_t *fsr,
+                             ARMMMUFaultInfo *fi)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
     int code;
@@ -6431,7 +6435,8 @@ typedef enum {
 static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
                                int access_type, ARMMMUIdx mmu_idx,
                                hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
-                               target_ulong *page_size_ptr, uint32_t *fsr)
+                               target_ulong *page_size_ptr, uint32_t *fsr,
+                               ARMMMUFaultInfo *fi)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
     /* Read an LPAE long-descriptor translation table. */
@@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           int access_type, ARMMMUIdx mmu_idx,
                           hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
-                          target_ulong *page_size, uint32_t *fsr)
+                          target_ulong *page_size, uint32_t *fsr,
+                          ARMMMUFaultInfo *fi)
 {
     if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
         /* TODO: when we support EL2 we should here call ourselves recursively
@@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
 
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
-                                  attrs, prot, page_size, fsr);
+                                  attrs, prot, page_size, fsr, fi);
     } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
         return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
-                                attrs, prot, page_size, fsr);
+                                attrs, prot, page_size, fsr, fi);
     } else {
         return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
-                                prot, page_size, fsr);
+                                prot, page_size, fsr, fi);
     }
 }
 
@@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
  * fsr with ARM DFSR/IFSR fault register format value on failure.
  */
 bool arm_tlb_fill(CPUState *cs, vaddr address,
-                  int access_type, int mmu_idx, uint32_t *fsr)
+                  int access_type, int mmu_idx, uint32_t *fsr,
+                  ARMMMUFaultInfo *fi)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
     MemTxAttrs attrs = {};
 
     ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
-                        &attrs, &prot, &page_size, fsr);
+                        &attrs, &prot, &page_size, fsr, fi);
     if (!ret) {
         /* Map a single [sub]page.  */
         phys_addr &= TARGET_PAGE_MASK;
@@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
     bool ret;
     uint32_t fsr;
     MemTxAttrs attrs = {};
+    ARMMMUFaultInfo fi = {};
 
     ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
-                        &attrs, &prot, &page_size, &fsr);
+                        &attrs, &prot, &page_size, &fsr, &fi);
 
     if (ret) {
         return -1;
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 36a56aa..3be23be 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
 void arm_handle_psci_call(ARMCPU *cpu);
 #endif
 
+/**
+ * ARMMMUFaultInfo: Information describing an ARM MMU Fault
+ * @s2addr: Address that caused a fault at stage 2
+ * @stage2: True if we faulted at stage 2
+ * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
+ */
+typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
+struct ARMMMUFaultInfo {
+    target_ulong s2addr;
+    bool stage2;
+    bool s1ptw;
+};
+
 /* Do a page table walk and add page to TLB if possible */
 bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
-                  uint32_t *fsr);
+                  uint32_t *fsr, ARMMMUFaultInfo *fi);
 
 #endif
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 1425a1d..7ff3c61 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -83,8 +83,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
 {
     bool ret;
     uint32_t fsr = 0;
+    struct ARMMMUFaultInfo fi = {0};
 
-    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr);
+    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr, &fi);
     if (unlikely(ret)) {
         ARMCPU *cpu = ARM_CPU(cs);
         CPUARMState *env = &cpu->env;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

[Alex Bennée]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Introduce ARMMMUFaultInfo to propagate MMU Fault information
> across the MMU translation code path. This is in preparation for
> adding State-2 translation.

s/State/stage/?

>
> No functional changes.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper.c    | 32 ++++++++++++++++++++------------
>  target-arm/internals.h | 15 ++++++++++++++-
>  target-arm/op_helper.c |  3 ++-
>  3 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index cbc1570..a429ff2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -18,7 +18,8 @@
>  static bool get_phys_addr(CPUARMState *env, target_ulong address,
>                            int access_type, ARMMMUIdx mmu_idx,
>                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                          target_ulong *page_size, uint32_t *fsr);
> +                          target_ulong *page_size, uint32_t *fsr,
> +                          ARMMMUFaultInfo *fi);
>  
>  /* Definitions for the PMCCNTR and PMCR registers */
>  #define PMCRD   0x8
> @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>      bool ret;
>      uint64_t par64;
>      MemTxAttrs attrs = {};
> +    ARMMMUFaultInfo fi = {};
>  
>      ret = get_phys_addr(env, value, access_type, mmu_idx,
> -                        &phys_addr, &attrs, &prot, &page_size, &fsr);
> +                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
>      if (extended_addresses_enabled(env)) {
>          /* fsr is a DFSR/IFSR value for the long descriptor
>           * translation table format, but with WnR always clear.
> @@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
>  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
>                               int access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, int *prot,
> -                             target_ulong *page_size, uint32_t *fsr)
> +                             target_ulong *page_size, uint32_t *fsr,
> +                             ARMMMUFaultInfo *fi)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      int code;
> @@ -6280,7 +6283,8 @@ do_fault:
>  static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
>                               int access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                             target_ulong *page_size, uint32_t *fsr)
> +                             target_ulong *page_size, uint32_t *fsr,
> +                             ARMMMUFaultInfo *fi)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      int code;
> @@ -6431,7 +6435,8 @@ typedef enum {
>  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>                                 int access_type, ARMMMUIdx mmu_idx,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
> -                               target_ulong *page_size_ptr, uint32_t *fsr)
> +                               target_ulong *page_size_ptr, uint32_t *fsr,
> +                               ARMMMUFaultInfo *fi)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      /* Read an LPAE long-descriptor translation table. */
> @@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>  static bool get_phys_addr(CPUARMState *env, target_ulong address,
>                            int access_type, ARMMMUIdx mmu_idx,
>                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                          target_ulong *page_size, uint32_t *fsr)
> +                          target_ulong *page_size, uint32_t *fsr,
> +                          ARMMMUFaultInfo *fi)
>  {
>      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
>          /* TODO: when we support EL2 we should here call ourselves recursively
> @@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>  
>      if (regime_using_lpae_format(env, mmu_idx)) {
>          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
> -                                  attrs, prot, page_size, fsr);
> +                                  attrs, prot, page_size, fsr, fi);
>      } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
>          return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
> -                                attrs, prot, page_size, fsr);
> +                                attrs, prot, page_size, fsr, fi);
>      } else {
>          return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
> -                                prot, page_size, fsr);
> +                                prot, page_size, fsr, fi);
>      }
>  }
>  
> @@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>   * fsr with ARM DFSR/IFSR fault register format value on failure.
>   */
>  bool arm_tlb_fill(CPUState *cs, vaddr address,
> -                  int access_type, int mmu_idx, uint32_t *fsr)
> +                  int access_type, int mmu_idx, uint32_t *fsr,
> +                  ARMMMUFaultInfo *fi)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>      MemTxAttrs attrs = {};
>  
>      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> -                        &attrs, &prot, &page_size, fsr);
> +                        &attrs, &prot, &page_size, fsr, fi);
>      if (!ret) {
>          /* Map a single [sub]page.  */
>          phys_addr &= TARGET_PAGE_MASK;
> @@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>      bool ret;
>      uint32_t fsr;
>      MemTxAttrs attrs = {};
> +    ARMMMUFaultInfo fi = {};
>  
>      ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
> -                        &attrs, &prot, &page_size, &fsr);
> +                        &attrs, &prot, &page_size, &fsr, &fi);
>  
>      if (ret) {
>          return -1;
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 36a56aa..3be23be 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
>  void arm_handle_psci_call(ARMCPU *cpu);
>  #endif
>  
> +/**
> + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
> + * @s2addr: Address that caused a fault at stage 2
> + * @stage2: True if we faulted at stage 2
> + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
> + */
> +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
> +struct ARMMMUFaultInfo {
> +    target_ulong s2addr;
> +    bool stage2;
> +    bool s1ptw;

I guess the compiler packs the bools down pretty well but why not just
encode the faulting stage in a single variable? Perhaps I'm
misunderstanding the potential combinations here.

> +};
> +
>  /* Do a page table walk and add page to TLB if possible */
>  bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
> -                  uint32_t *fsr);
> +                  uint32_t *fsr, ARMMMUFaultInfo *fi);
>  
>  #endif
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 1425a1d..7ff3c61 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -83,8 +83,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>  {
>      bool ret;
>      uint32_t fsr = 0;
> +    struct ARMMMUFaultInfo fi = {0};
>  
> -    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr);
> +    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr, &fi);
>      if (unlikely(ret)) {
>          ARMCPU *cpu = ARM_CPU(cs);
>          CPUARMState *env = &cpu->env;

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

[Edgar E. Iglesias]

On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Introduce ARMMMUFaultInfo to propagate MMU Fault information
> > across the MMU translation code path. This is in preparation for
> > adding State-2 translation.
> 
> s/State/stage/?


Thanks, I've fixed this


> 
> >
> > No functional changes.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c    | 32 ++++++++++++++++++++------------
> >  target-arm/internals.h | 15 ++++++++++++++-
> >  target-arm/op_helper.c |  3 ++-
> >  3 files changed, 36 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index cbc1570..a429ff2 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -18,7 +18,8 @@
> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >                            int access_type, ARMMMUIdx mmu_idx,
> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> > -                          target_ulong *page_size, uint32_t *fsr);
> > +                          target_ulong *page_size, uint32_t *fsr,
> > +                          ARMMMUFaultInfo *fi);
> >  
> >  /* Definitions for the PMCCNTR and PMCR registers */
> >  #define PMCRD   0x8
> > @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> >      bool ret;
> >      uint64_t par64;
> >      MemTxAttrs attrs = {};
> > +    ARMMMUFaultInfo fi = {};
> >  
> >      ret = get_phys_addr(env, value, access_type, mmu_idx,
> > -                        &phys_addr, &attrs, &prot, &page_size, &fsr);
> > +                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
> >      if (extended_addresses_enabled(env)) {
> >          /* fsr is a DFSR/IFSR value for the long descriptor
> >           * translation table format, but with WnR always clear.
> > @@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
> >  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> >                               int access_type, ARMMMUIdx mmu_idx,
> >                               hwaddr *phys_ptr, int *prot,
> > -                             target_ulong *page_size, uint32_t *fsr)
> > +                             target_ulong *page_size, uint32_t *fsr,
> > +                             ARMMMUFaultInfo *fi)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >      int code;
> > @@ -6280,7 +6283,8 @@ do_fault:
> >  static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
> >                               int access_type, ARMMMUIdx mmu_idx,
> >                               hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> > -                             target_ulong *page_size, uint32_t *fsr)
> > +                             target_ulong *page_size, uint32_t *fsr,
> > +                             ARMMMUFaultInfo *fi)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >      int code;
> > @@ -6431,7 +6435,8 @@ typedef enum {
> >  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >                                 int access_type, ARMMMUIdx mmu_idx,
> >                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
> > -                               target_ulong *page_size_ptr, uint32_t *fsr)
> > +                               target_ulong *page_size_ptr, uint32_t *fsr,
> > +                               ARMMMUFaultInfo *fi)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >      /* Read an LPAE long-descriptor translation table. */
> > @@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >                            int access_type, ARMMMUIdx mmu_idx,
> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> > -                          target_ulong *page_size, uint32_t *fsr)
> > +                          target_ulong *page_size, uint32_t *fsr,
> > +                          ARMMMUFaultInfo *fi)
> >  {
> >      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> >          /* TODO: when we support EL2 we should here call ourselves recursively
> > @@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >  
> >      if (regime_using_lpae_format(env, mmu_idx)) {
> >          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
> > -                                  attrs, prot, page_size, fsr);
> > +                                  attrs, prot, page_size, fsr, fi);
> >      } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> >          return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
> > -                                attrs, prot, page_size, fsr);
> > +                                attrs, prot, page_size, fsr, fi);
> >      } else {
> >          return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
> > -                                prot, page_size, fsr);
> > +                                prot, page_size, fsr, fi);
> >      }
> >  }
> >  
> > @@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >   * fsr with ARM DFSR/IFSR fault register format value on failure.
> >   */
> >  bool arm_tlb_fill(CPUState *cs, vaddr address,
> > -                  int access_type, int mmu_idx, uint32_t *fsr)
> > +                  int access_type, int mmu_idx, uint32_t *fsr,
> > +                  ARMMMUFaultInfo *fi)
> >  {
> >      ARMCPU *cpu = ARM_CPU(cs);
> >      CPUARMState *env = &cpu->env;
> > @@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
> >      MemTxAttrs attrs = {};
> >  
> >      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> > -                        &attrs, &prot, &page_size, fsr);
> > +                        &attrs, &prot, &page_size, fsr, fi);
> >      if (!ret) {
> >          /* Map a single [sub]page.  */
> >          phys_addr &= TARGET_PAGE_MASK;
> > @@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> >      bool ret;
> >      uint32_t fsr;
> >      MemTxAttrs attrs = {};
> > +    ARMMMUFaultInfo fi = {};
> >  
> >      ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
> > -                        &attrs, &prot, &page_size, &fsr);
> > +                        &attrs, &prot, &page_size, &fsr, &fi);
> >  
> >      if (ret) {
> >          return -1;
> > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > index 36a56aa..3be23be 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
> >  void arm_handle_psci_call(ARMCPU *cpu);
> >  #endif
> >  
> > +/**
> > + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
> > + * @s2addr: Address that caused a fault at stage 2
> > + * @stage2: True if we faulted at stage 2
> > + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
> > + */
> > +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
> > +struct ARMMMUFaultInfo {
> > +    target_ulong s2addr;
> > +    bool stage2;
> > +    bool s1ptw;
> 
> I guess the compiler packs the bools down pretty well but why not just
> encode the faulting stage in a single variable? Perhaps I'm
> misunderstanding the potential combinations here.
> 
> > +};
> > +
> >  /* Do a page table walk and add page to TLB if possible */
> >  bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
> > -                  uint32_t *fsr);
> > +                  uint32_t *fsr, ARMMMUFaultInfo *fi);
> >  
> >  #endif
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index 1425a1d..7ff3c61 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -83,8 +83,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> >  {
> >      bool ret;
> >      uint32_t fsr = 0;
> > +    struct ARMMMUFaultInfo fi = {0};
> >  
> > -    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr);
> > +    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr, &fi);
> >      if (unlikely(ret)) {
> >          ARMCPU *cpu = ARM_CPU(cs);
> >          CPUARMState *env = &cpu->env;
> 
> -- 
> Alex Bennée

Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

[Edgar E. Iglesias]

On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Introduce ARMMMUFaultInfo to propagate MMU Fault information
> > across the MMU translation code path. This is in preparation for
> > adding State-2 translation.
> 
> s/State/stage/?
> 
> >
> > No functional changes.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/helper.c    | 32 ++++++++++++++++++++------------
> >  target-arm/internals.h | 15 ++++++++++++++-
> >  target-arm/op_helper.c |  3 ++-
> >  3 files changed, 36 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index cbc1570..a429ff2 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -18,7 +18,8 @@
> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >                            int access_type, ARMMMUIdx mmu_idx,
> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> > -                          target_ulong *page_size, uint32_t *fsr);
> > +                          target_ulong *page_size, uint32_t *fsr,
> > +                          ARMMMUFaultInfo *fi);
> >  
> >  /* Definitions for the PMCCNTR and PMCR registers */
> >  #define PMCRD   0x8
> > @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> >      bool ret;
> >      uint64_t par64;
> >      MemTxAttrs attrs = {};
> > +    ARMMMUFaultInfo fi = {};
> >  
> >      ret = get_phys_addr(env, value, access_type, mmu_idx,
> > -                        &phys_addr, &attrs, &prot, &page_size, &fsr);
> > +                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
> >      if (extended_addresses_enabled(env)) {
> >          /* fsr is a DFSR/IFSR value for the long descriptor
> >           * translation table format, but with WnR always clear.
> > @@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
> >  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> >                               int access_type, ARMMMUIdx mmu_idx,
> >                               hwaddr *phys_ptr, int *prot,
> > -                             target_ulong *page_size, uint32_t *fsr)
> > +                             target_ulong *page_size, uint32_t *fsr,
> > +                             ARMMMUFaultInfo *fi)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >      int code;
> > @@ -6280,7 +6283,8 @@ do_fault:
> >  static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
> >                               int access_type, ARMMMUIdx mmu_idx,
> >                               hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> > -                             target_ulong *page_size, uint32_t *fsr)
> > +                             target_ulong *page_size, uint32_t *fsr,
> > +                             ARMMMUFaultInfo *fi)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >      int code;
> > @@ -6431,7 +6435,8 @@ typedef enum {
> >  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >                                 int access_type, ARMMMUIdx mmu_idx,
> >                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
> > -                               target_ulong *page_size_ptr, uint32_t *fsr)
> > +                               target_ulong *page_size_ptr, uint32_t *fsr,
> > +                               ARMMMUFaultInfo *fi)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >      /* Read an LPAE long-descriptor translation table. */
> > @@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >                            int access_type, ARMMMUIdx mmu_idx,
> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> > -                          target_ulong *page_size, uint32_t *fsr)
> > +                          target_ulong *page_size, uint32_t *fsr,
> > +                          ARMMMUFaultInfo *fi)
> >  {
> >      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> >          /* TODO: when we support EL2 we should here call ourselves recursively
> > @@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >  
> >      if (regime_using_lpae_format(env, mmu_idx)) {
> >          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
> > -                                  attrs, prot, page_size, fsr);
> > +                                  attrs, prot, page_size, fsr, fi);
> >      } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> >          return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
> > -                                attrs, prot, page_size, fsr);
> > +                                attrs, prot, page_size, fsr, fi);
> >      } else {
> >          return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
> > -                                prot, page_size, fsr);
> > +                                prot, page_size, fsr, fi);
> >      }
> >  }
> >  
> > @@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >   * fsr with ARM DFSR/IFSR fault register format value on failure.
> >   */
> >  bool arm_tlb_fill(CPUState *cs, vaddr address,
> > -                  int access_type, int mmu_idx, uint32_t *fsr)
> > +                  int access_type, int mmu_idx, uint32_t *fsr,
> > +                  ARMMMUFaultInfo *fi)
> >  {
> >      ARMCPU *cpu = ARM_CPU(cs);
> >      CPUARMState *env = &cpu->env;
> > @@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
> >      MemTxAttrs attrs = {};
> >  
> >      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> > -                        &attrs, &prot, &page_size, fsr);
> > +                        &attrs, &prot, &page_size, fsr, fi);
> >      if (!ret) {
> >          /* Map a single [sub]page.  */
> >          phys_addr &= TARGET_PAGE_MASK;
> > @@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> >      bool ret;
> >      uint32_t fsr;
> >      MemTxAttrs attrs = {};
> > +    ARMMMUFaultInfo fi = {};
> >  
> >      ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
> > -                        &attrs, &prot, &page_size, &fsr);
> > +                        &attrs, &prot, &page_size, &fsr, &fi);
> >  
> >      if (ret) {
> >          return -1;
> > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > index 36a56aa..3be23be 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
> >  void arm_handle_psci_call(ARMCPU *cpu);
> >  #endif
> >  
> > +/**
> > + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
> > + * @s2addr: Address that caused a fault at stage 2
> > + * @stage2: True if we faulted at stage 2
> > + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
> > + */
> > +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
> > +struct ARMMMUFaultInfo {
> > +    target_ulong s2addr;
> > +    bool stage2;
> > +    bool s1ptw;
> 
> I guess the compiler packs the bools down pretty well but why not just
> encode the faulting stage in a single variable? Perhaps I'm
> misunderstanding the potential combinations here.
> 

Do you mean using bitfields?
e.g:

struct ARMMMUFaultInfo {
    target_ulong s2addr;
    unsigned int stage2 : 1;
    unsigned int s1ptw : 1;
}
If so, I guess we could. If others agree, I can change to that.
We could also maybe structure things differently. I didn't consider
the encodings very much here to be honest...

Or do you mean why we need these at all?
We need it because the places in the code where we figure out that
a fault occurs don't have paths to propagate details about the error
back to the callers that later need to generate the exception syndromes.

The s1ptw field is additional detail to a stage-2 fault describing that
the stage-2 fault happened while doing a page-table walk for a stage-1
translation. s2addr is similar additional info to a stage-2 fault,
passing the fault address so we can report it via the hypervisor IPA
fault address reg.

Best regards,
Edgar

Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

[Peter Maydell]

On 8 October 2015 at 21:06, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote:
>>
>> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>>
>> > +/**
>> > + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
>> > + * @s2addr: Address that caused a fault at stage 2
>> > + * @stage2: True if we faulted at stage 2
>> > + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
>> > + */
>> > +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
>> > +struct ARMMMUFaultInfo {
>> > +    target_ulong s2addr;
>> > +    bool stage2;
>> > +    bool s1ptw;
>>
>> I guess the compiler packs the bools down pretty well but why not just
>> encode the faulting stage in a single variable? Perhaps I'm
>> misunderstanding the potential combinations here.
>>
>
> Do you mean using bitfields?
> e.g:
>
> struct ARMMMUFaultInfo {
>     target_ulong s2addr;
>     unsigned int stage2 : 1;
>     unsigned int s1ptw : 1;
> }
> If so, I guess we could. If others agree, I can change to that.
> We could also maybe structure things differently. I didn't consider
> the encodings very much here to be honest...

I'm not a great fan of bitfields personally; I think a pair
of bool fields expresses things more clearly.

-- PMM

Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

[Alex Bennée]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote:
>> 
>> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>> 
>> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>> >
>> > Introduce ARMMMUFaultInfo to propagate MMU Fault information
>> > across the MMU translation code path. This is in preparation for
>> > adding State-2 translation.
>> 
>> s/State/stage/?
>> 
>> >
>> > No functional changes.
>> >
>> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> > ---
>> >  target-arm/helper.c    | 32 ++++++++++++++++++++------------
>> >  target-arm/internals.h | 15 ++++++++++++++-
>> >  target-arm/op_helper.c |  3 ++-
>> >  3 files changed, 36 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/target-arm/helper.c b/target-arm/helper.c
>> > index cbc1570..a429ff2 100644
>> > --- a/target-arm/helper.c
>> > +++ b/target-arm/helper.c
>> > @@ -18,7 +18,8 @@
>> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
>> >                            int access_type, ARMMMUIdx mmu_idx,
>> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>> > -                          target_ulong *page_size, uint32_t *fsr);
>> > +                          target_ulong *page_size, uint32_t *fsr,
>> > +                          ARMMMUFaultInfo *fi);
>> >  
>> >  /* Definitions for the PMCCNTR and PMCR registers */
>> >  #define PMCRD   0x8
>> > @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>> >      bool ret;
>> >      uint64_t par64;
>> >      MemTxAttrs attrs = {};
>> > +    ARMMMUFaultInfo fi = {};
>> >  
>> >      ret = get_phys_addr(env, value, access_type, mmu_idx,
>> > -                        &phys_addr, &attrs, &prot, &page_size, &fsr);
>> > +                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
>> >      if (extended_addresses_enabled(env)) {
>> >          /* fsr is a DFSR/IFSR value for the long descriptor
>> >           * translation table format, but with WnR always clear.
>> > @@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
>> >  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
>> >                               int access_type, ARMMMUIdx mmu_idx,
>> >                               hwaddr *phys_ptr, int *prot,
>> > -                             target_ulong *page_size, uint32_t *fsr)
>> > +                             target_ulong *page_size, uint32_t *fsr,
>> > +                             ARMMMUFaultInfo *fi)
>> >  {
>> >      CPUState *cs = CPU(arm_env_get_cpu(env));
>> >      int code;
>> > @@ -6280,7 +6283,8 @@ do_fault:
>> >  static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
>> >                               int access_type, ARMMMUIdx mmu_idx,
>> >                               hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>> > -                             target_ulong *page_size, uint32_t *fsr)
>> > +                             target_ulong *page_size, uint32_t *fsr,
>> > +                             ARMMMUFaultInfo *fi)
>> >  {
>> >      CPUState *cs = CPU(arm_env_get_cpu(env));
>> >      int code;
>> > @@ -6431,7 +6435,8 @@ typedef enum {
>> >  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>> >                                 int access_type, ARMMMUIdx mmu_idx,
>> >                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
>> > -                               target_ulong *page_size_ptr, uint32_t *fsr)
>> > +                               target_ulong *page_size_ptr, uint32_t *fsr,
>> > +                               ARMMMUFaultInfo *fi)
>> >  {
>> >      CPUState *cs = CPU(arm_env_get_cpu(env));
>> >      /* Read an LPAE long-descriptor translation table. */
>> > @@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
>> >                            int access_type, ARMMMUIdx mmu_idx,
>> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>> > -                          target_ulong *page_size, uint32_t *fsr)
>> > +                          target_ulong *page_size, uint32_t *fsr,
>> > +                          ARMMMUFaultInfo *fi)
>> >  {
>> >      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
>> >          /* TODO: when we support EL2 we should here call ourselves recursively
>> > @@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>> >  
>> >      if (regime_using_lpae_format(env, mmu_idx)) {
>> >          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
>> > -                                  attrs, prot, page_size, fsr);
>> > +                                  attrs, prot, page_size, fsr, fi);
>> >      } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
>> >          return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
>> > -                                attrs, prot, page_size, fsr);
>> > +                                attrs, prot, page_size, fsr, fi);
>> >      } else {
>> >          return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
>> > -                                prot, page_size, fsr);
>> > +                                prot, page_size, fsr, fi);
>> >      }
>> >  }
>> >  
>> > @@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>> >   * fsr with ARM DFSR/IFSR fault register format value on failure.
>> >   */
>> >  bool arm_tlb_fill(CPUState *cs, vaddr address,
>> > -                  int access_type, int mmu_idx, uint32_t *fsr)
>> > +                  int access_type, int mmu_idx, uint32_t *fsr,
>> > +                  ARMMMUFaultInfo *fi)
>> >  {
>> >      ARMCPU *cpu = ARM_CPU(cs);
>> >      CPUARMState *env = &cpu->env;
>> > @@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>> >      MemTxAttrs attrs = {};
>> >  
>> >      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
>> > -                        &attrs, &prot, &page_size, fsr);
>> > +                        &attrs, &prot, &page_size, fsr, fi);
>> >      if (!ret) {
>> >          /* Map a single [sub]page.  */
>> >          phys_addr &= TARGET_PAGE_MASK;
>> > @@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>> >      bool ret;
>> >      uint32_t fsr;
>> >      MemTxAttrs attrs = {};
>> > +    ARMMMUFaultInfo fi = {};
>> >  
>> >      ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
>> > -                        &attrs, &prot, &page_size, &fsr);
>> > +                        &attrs, &prot, &page_size, &fsr, &fi);
>> >  
>> >      if (ret) {
>> >          return -1;
>> > diff --git a/target-arm/internals.h b/target-arm/internals.h
>> > index 36a56aa..3be23be 100644
>> > --- a/target-arm/internals.h
>> > +++ b/target-arm/internals.h
>> > @@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
>> >  void arm_handle_psci_call(ARMCPU *cpu);
>> >  #endif
>> >  
>> > +/**
>> > + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
>> > + * @s2addr: Address that caused a fault at stage 2
>> > + * @stage2: True if we faulted at stage 2
>> > + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
>> > + */
>> > +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
>> > +struct ARMMMUFaultInfo {
>> > +    target_ulong s2addr;
>> > +    bool stage2;
>> > +    bool s1ptw;
>> 
>> I guess the compiler packs the bools down pretty well but why not just
>> encode the faulting stage in a single variable? Perhaps I'm
>> misunderstanding the potential combinations here.
>> 
>
> Do you mean using bitfields?
> e.g:
>
> struct ARMMMUFaultInfo {
>     target_ulong s2addr;
>     unsigned int stage2 : 1;
>     unsigned int s1ptw : 1;
> }
> If so, I guess we could. If others agree, I can change to that.
> We could also maybe structure things differently. I didn't consider
> the encodings very much here to be honest...

No I didn't. 

>
> Or do you mean why we need these at all?
> We need it because the places in the code where we figure out that
> a fault occurs don't have paths to propagate details about the error
> back to the callers that later need to generate the exception syndromes.
>
> The s1ptw field is additional detail to a stage-2 fault describing that
> the stage-2 fault happened while doing a page-table walk for a stage-1
> translation. s2addr is similar additional info to a stage-2 fault,
> passing the fault address so we can report it via the hypervisor IPA
> fault address reg.

My concern was making the meaning of the various variables intuitive.
>From what I can tell this info is only relevant to stage 2 faults which
may occur while walking stage 1 and stage 2 page tables? Will we ever
see a point where s2addr = NULL, stage2 = false and s1ptw = false?

I'll come back to this once I've gone over the later patches to see if
it makes more sense.

>
> Best regards,
> Edgar

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo

[Edgar E. Iglesias]

On Wed, Oct 14, 2015 at 02:00:17PM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
> 
> > On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote:
> >> 
> >> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
> >> 
> >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >> >
> >> > Introduce ARMMMUFaultInfo to propagate MMU Fault information
> >> > across the MMU translation code path. This is in preparation for
> >> > adding State-2 translation.
> >> 
> >> s/State/stage/?
> >> 
> >> >
> >> > No functional changes.
> >> >
> >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >> > ---
> >> >  target-arm/helper.c    | 32 ++++++++++++++++++++------------
> >> >  target-arm/internals.h | 15 ++++++++++++++-
> >> >  target-arm/op_helper.c |  3 ++-
> >> >  3 files changed, 36 insertions(+), 14 deletions(-)
> >> >
> >> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> >> > index cbc1570..a429ff2 100644
> >> > --- a/target-arm/helper.c
> >> > +++ b/target-arm/helper.c
> >> > @@ -18,7 +18,8 @@
> >> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >> >                            int access_type, ARMMMUIdx mmu_idx,
> >> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> >> > -                          target_ulong *page_size, uint32_t *fsr);
> >> > +                          target_ulong *page_size, uint32_t *fsr,
> >> > +                          ARMMMUFaultInfo *fi);
> >> >  
> >> >  /* Definitions for the PMCCNTR and PMCR registers */
> >> >  #define PMCRD   0x8
> >> > @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> >> >      bool ret;
> >> >      uint64_t par64;
> >> >      MemTxAttrs attrs = {};
> >> > +    ARMMMUFaultInfo fi = {};
> >> >  
> >> >      ret = get_phys_addr(env, value, access_type, mmu_idx,
> >> > -                        &phys_addr, &attrs, &prot, &page_size, &fsr);
> >> > +                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
> >> >      if (extended_addresses_enabled(env)) {
> >> >          /* fsr is a DFSR/IFSR value for the long descriptor
> >> >           * translation table format, but with WnR always clear.
> >> > @@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
> >> >  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> >> >                               int access_type, ARMMMUIdx mmu_idx,
> >> >                               hwaddr *phys_ptr, int *prot,
> >> > -                             target_ulong *page_size, uint32_t *fsr)
> >> > +                             target_ulong *page_size, uint32_t *fsr,
> >> > +                             ARMMMUFaultInfo *fi)
> >> >  {
> >> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >> >      int code;
> >> > @@ -6280,7 +6283,8 @@ do_fault:
> >> >  static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
> >> >                               int access_type, ARMMMUIdx mmu_idx,
> >> >                               hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> >> > -                             target_ulong *page_size, uint32_t *fsr)
> >> > +                             target_ulong *page_size, uint32_t *fsr,
> >> > +                             ARMMMUFaultInfo *fi)
> >> >  {
> >> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >> >      int code;
> >> > @@ -6431,7 +6435,8 @@ typedef enum {
> >> >  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >> >                                 int access_type, ARMMMUIdx mmu_idx,
> >> >                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
> >> > -                               target_ulong *page_size_ptr, uint32_t *fsr)
> >> > +                               target_ulong *page_size_ptr, uint32_t *fsr,
> >> > +                               ARMMMUFaultInfo *fi)
> >> >  {
> >> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> >> >      /* Read an LPAE long-descriptor translation table. */
> >> > @@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
> >> >  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >> >                            int access_type, ARMMMUIdx mmu_idx,
> >> >                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> >> > -                          target_ulong *page_size, uint32_t *fsr)
> >> > +                          target_ulong *page_size, uint32_t *fsr,
> >> > +                          ARMMMUFaultInfo *fi)
> >> >  {
> >> >      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> >> >          /* TODO: when we support EL2 we should here call ourselves recursively
> >> > @@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >> >  
> >> >      if (regime_using_lpae_format(env, mmu_idx)) {
> >> >          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
> >> > -                                  attrs, prot, page_size, fsr);
> >> > +                                  attrs, prot, page_size, fsr, fi);
> >> >      } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> >> >          return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
> >> > -                                attrs, prot, page_size, fsr);
> >> > +                                attrs, prot, page_size, fsr, fi);
> >> >      } else {
> >> >          return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
> >> > -                                prot, page_size, fsr);
> >> > +                                prot, page_size, fsr, fi);
> >> >      }
> >> >  }
> >> >  
> >> > @@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
> >> >   * fsr with ARM DFSR/IFSR fault register format value on failure.
> >> >   */
> >> >  bool arm_tlb_fill(CPUState *cs, vaddr address,
> >> > -                  int access_type, int mmu_idx, uint32_t *fsr)
> >> > +                  int access_type, int mmu_idx, uint32_t *fsr,
> >> > +                  ARMMMUFaultInfo *fi)
> >> >  {
> >> >      ARMCPU *cpu = ARM_CPU(cs);
> >> >      CPUARMState *env = &cpu->env;
> >> > @@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
> >> >      MemTxAttrs attrs = {};
> >> >  
> >> >      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> >> > -                        &attrs, &prot, &page_size, fsr);
> >> > +                        &attrs, &prot, &page_size, fsr, fi);
> >> >      if (!ret) {
> >> >          /* Map a single [sub]page.  */
> >> >          phys_addr &= TARGET_PAGE_MASK;
> >> > @@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> >> >      bool ret;
> >> >      uint32_t fsr;
> >> >      MemTxAttrs attrs = {};
> >> > +    ARMMMUFaultInfo fi = {};
> >> >  
> >> >      ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false), &phys_addr,
> >> > -                        &attrs, &prot, &page_size, &fsr);
> >> > +                        &attrs, &prot, &page_size, &fsr, &fi);
> >> >  
> >> >      if (ret) {
> >> >          return -1;
> >> > diff --git a/target-arm/internals.h b/target-arm/internals.h
> >> > index 36a56aa..3be23be 100644
> >> > --- a/target-arm/internals.h
> >> > +++ b/target-arm/internals.h
> >> > @@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
> >> >  void arm_handle_psci_call(ARMCPU *cpu);
> >> >  #endif
> >> >  
> >> > +/**
> >> > + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
> >> > + * @s2addr: Address that caused a fault at stage 2
> >> > + * @stage2: True if we faulted at stage 2
> >> > + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
> >> > + */
> >> > +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
> >> > +struct ARMMMUFaultInfo {
> >> > +    target_ulong s2addr;
> >> > +    bool stage2;
> >> > +    bool s1ptw;
> >> 
> >> I guess the compiler packs the bools down pretty well but why not just
> >> encode the faulting stage in a single variable? Perhaps I'm
> >> misunderstanding the potential combinations here.
> >> 
> >
> > Do you mean using bitfields?
> > e.g:
> >
> > struct ARMMMUFaultInfo {
> >     target_ulong s2addr;
> >     unsigned int stage2 : 1;
> >     unsigned int s1ptw : 1;
> > }
> > If so, I guess we could. If others agree, I can change to that.
> > We could also maybe structure things differently. I didn't consider
> > the encodings very much here to be honest...
> 
> No I didn't. 
> 
> >
> > Or do you mean why we need these at all?
> > We need it because the places in the code where we figure out that
> > a fault occurs don't have paths to propagate details about the error
> > back to the callers that later need to generate the exception syndromes.
> >
> > The s1ptw field is additional detail to a stage-2 fault describing that
> > the stage-2 fault happened while doing a page-table walk for a stage-1
> > translation. s2addr is similar additional info to a stage-2 fault,
> > passing the fault address so we can report it via the hypervisor IPA
> > fault address reg.
> 
> My concern was making the meaning of the various variables intuitive.
> From what I can tell this info is only relevant to stage 2 faults which
> may occur while walking stage 1 and stage 2 page tables? Will we ever
> see a point where s2addr = NULL, stage2 = false and s1ptw = false?

A pure stage1 fault would render that. I guess we could throw in more
stuff in here (e.g fsr) but I'm also trying to keep this series small
to avoid too much change from creeping in.

I'll be sending a v4 tonight BTW.

Cheers,
Edgar

[Qemu-devel] [PATCH v3 6/9] target-arm: Add S2 translation to 64bit S1 PTWs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for applying S2 translation to 64bit S1
page-table walks.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c    | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/op_helper.c |  4 ++--
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index a429ff2..fca2aef 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -21,6 +21,12 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           target_ulong *page_size, uint32_t *fsr,
                           ARMMMUFaultInfo *fi);
 
+static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
+                               int access_type, ARMMMUIdx mmu_idx,
+                               hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
+                               target_ulong *page_size_ptr, uint32_t *fsr,
+                               ARMMMUFaultInfo *fi);
+
 /* Definitions for the PMCCNTR and PMCR registers */
 #define PMCRD   0x8
 #define PMCRC   0x4
@@ -6143,6 +6149,32 @@ static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
     return true;
 }
 
+/* Translate a S1 pagetable walk through S2 if needed.  */
+static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
+                               hwaddr addr, MemTxAttrs txattrs,
+                               uint32_t *fsr,
+                               ARMMMUFaultInfo *fi)
+{
+    if ((mmu_idx == ARMMMUIdx_S1NSE0 || mmu_idx == ARMMMUIdx_S1NSE1) &&
+        !regime_translation_disabled(env, ARMMMUIdx_S2NS)) {
+        target_ulong s2size;
+        hwaddr s2pa;
+        int s2prot;
+        int ret;
+
+        ret = get_phys_addr_lpae(env, addr, 0, ARMMMUIdx_S2NS, &s2pa,
+                                 &txattrs, &s2prot, &s2size, fsr, fi);
+        if (ret) {
+            fi->s2addr = addr;
+            fi->stage2 = true;
+            fi->s1ptw = true;
+            return ~0;
+        }
+        addr = s2pa;
+    }
+    return addr;
+}
+
 /* All loads done in the course of a page table walk go through here.
  * TODO: rather than ignoring errors from physical memory reads (which
  * are external aborts in ARM terminology) we should propagate this
@@ -6158,11 +6190,19 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure)
     return address_space_ldl(cs->as, addr, attrs, NULL);
 }
 
-static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure)
+static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
+                            ARMMMUIdx mmu_idx, uint32_t *fsr,
+                            ARMMMUFaultInfo *fi)
 {
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
 
     attrs.secure = is_secure;
+    addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
+    if (fi->s1ptw) {
+        return 0;
+    }
     return address_space_ldq(cs->as, addr, attrs, NULL);
 }
 
@@ -6631,7 +6671,11 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         descaddr |= (address >> (granule_sz * (4 - level))) & descmask;
         descaddr &= ~7ULL;
         nstable = extract32(tableattrs, 4, 1);
-        descriptor = arm_ldq_ptw(cs, descaddr, !nstable);
+        descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
+        if (fi->s1ptw) {
+            goto do_fault;
+        }
+
         if (!(descriptor & 1) ||
             (!(descriptor & 2) && (level == 3))) {
             /* Invalid, or the Reserved level 3 encoding */
@@ -6715,6 +6759,8 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
 do_fault:
     /* Long-descriptor format IFSR/DFSR value */
     *fsr = (1 << 9) | (fault_type << 2) | level;
+    /* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2.  */
+    fi->stage2 = fi->s1ptw || (mmu_idx == ARMMMUIdx_S2NS);
     return true;
 }
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 7ff3c61..d4715f4 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -104,10 +104,10 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
          * information; this is always true for exceptions reported to EL1.
          */
         if (is_write == 2) {
-            syn = syn_insn_abort(same_el, 0, 0, syn);
+            syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
             exc = EXCP_PREFETCH_ABORT;
         } else {
-            syn = syn_data_abort(same_el, 0, 0, 0, is_write == 1, syn);
+            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
             }
-- 
1.9.1

[Qemu-devel] [PATCH v3 7/9] target-arm: Add S2 translation to 32bit S1 PTWs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for applying S2 translation to 32bit S1
page-table walks.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index fca2aef..c108248 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6182,11 +6182,19 @@ static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
  * was being done for a CPU load/store or an address translation instruction
  * (but not if it was for a debug access).
  */
-static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure)
+static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
+                            ARMMMUIdx mmu_idx, uint32_t *fsr,
+                            ARMMMUFaultInfo *fi)
 {
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
 
     attrs.secure = is_secure;
+    addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
+    if (fi->s1ptw) {
+        return 0;
+    }
     return address_space_ldl(cs->as, addr, attrs, NULL);
 }
 
@@ -6230,7 +6238,8 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
         code = 5;
         goto do_fault;
     }
-    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                       mmu_idx, fsr, fi);
     type = (desc & 3);
     domain = (desc >> 5) & 0x0f;
     if (regime_el(env, mmu_idx) == 1) {
@@ -6266,7 +6275,8 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
             /* Fine pagetable.  */
             table = (desc & 0xfffff000) | ((address >> 8) & 0xffc);
         }
-        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                           mmu_idx, fsr, fi);
         switch (desc & 3) {
         case 0: /* Page translation fault.  */
             code = 7;
@@ -6347,7 +6357,8 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
         code = 5;
         goto do_fault;
     }
-    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+    desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                       mmu_idx, fsr, fi);
     type = (desc & 3);
     if (type == 0 || (type == 3 && !arm_feature(env, ARM_FEATURE_PXN))) {
         /* Section translation fault, or attempt to use the encoding
@@ -6398,7 +6409,8 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
         ns = extract32(desc, 3, 1);
         /* Lookup l2 entry.  */
         table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
-        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx));
+        desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
+                           mmu_idx, fsr, fi);
         ap = ((desc >> 4) & 3) | ((desc >> 7) & 4);
         switch (desc & 3) {
         case 0: /* Page translation fault.  */
-- 
1.9.1

[Qemu-devel] [PATCH v3 8/9] target-arm: Route S2 MMU faults to EL2

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/op_helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index d4715f4..2ccd1c9 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -90,13 +90,19 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
         ARMCPU *cpu = ARM_CPU(cs);
         CPUARMState *env = &cpu->env;
         uint32_t syn, exc;
-        bool same_el = (arm_current_el(env) != 0);
+        unsigned int target_el;
+        bool same_el;
 
         if (retaddr) {
             /* now we have a real cpu fault */
             cpu_restore_state(cs, retaddr);
         }
 
+        target_el = exception_target_el(env);
+        if (fi.stage2) {
+            target_el = 2;
+        }
+        same_el = arm_current_el(env) == target_el;
         /* AArch64 syndrome does not have an LPAE bit */
         syn = fsr & ~(1 << 9);
 
@@ -116,7 +122,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
 
         env->exception.vaddress = addr;
         env->exception.fsr = fsr;
-        raise_exception(env, exc, syn, exception_target_el(env));
+        raise_exception(env, exc, syn, target_el);
     }
 }
 #endif
-- 
1.9.1

[Qemu-devel] [PATCH v3 9/9] target-arm: Add support for S1 + S2 MMU translations

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index c108248..0ec5909 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7042,14 +7042,44 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           ARMMMUFaultInfo *fi)
 {
     if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
-        /* TODO: when we support EL2 we should here call ourselves recursively
-         * to do the stage 1 and then stage 2 translations. The arm_ld*_ptw
-         * functions will also need changing to perform ARMMMUIdx_S2NS loads
-         * rather than direct physical memory loads when appropriate.
-         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
+        /* Call ourselves recursively to do the stage 1 and then stage 2
+         * translations.
          */
-        assert(!arm_feature(env, ARM_FEATURE_EL2));
-        mmu_idx += ARMMMUIdx_S1NSE0;
+        if (arm_feature(env, ARM_FEATURE_EL2)) {
+            hwaddr ipa;
+            int s2_prot;
+            int ret;
+
+            ret = get_phys_addr(env, address, access_type,
+                                mmu_idx + ARMMMUIdx_S1NSE0, &ipa, attrs,
+                                prot, page_size, fsr, fi);
+
+            /* If S1 fails or S2 is disabled, return early.  */
+            if (ret || regime_translation_disabled(env, ARMMMUIdx_S2NS)) {
+                if (ret && fi->stage2) {
+                    /* This is a S2 error while doing S1 PTW.  */
+                    env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;
+                }
+                *phys_ptr = ipa;
+                return ret;
+            }
+
+            /* S1 is done. Now do S2 translation.  */
+            ret = get_phys_addr_lpae(env, ipa, access_type, ARMMMUIdx_S2NS,
+                                     phys_ptr, attrs, &s2_prot,
+                                     page_size, fsr, fi);
+            if (ret) {
+                env->cp15.hpfar_el2 = extract64(ipa, 12, 47) << 4;
+            }
+            /* Combine the S1 and S2 perms.  */
+            *prot &= s2_prot;
+            return ret;
+        } else {
+            /*
+             * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
+             */
+            mmu_idx += ARMMMUIdx_S1NSE0;
+        }
     }
 
     /* The page table entries may downgrade secure to non-secure, but
-- 
1.9.1