Re: [Qemu-devel] [PATCH v8 11/27] target-arm: add SDER definition

[Greg Bellows]

On 31 October 2014 08:30, Peter Maydell <address@hidden> wrote:

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> From: Sergey Fedorov <address@hidden>
>
> Added CP register defintions for SDER and SDER32_EL3 as well as cp15.sder for
> register storage.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
>
> v7 -> v8
> - Added SDER32_EL3 register definition
> - Changed sder name from c1_sder to sder
> - Changed sder from uint32_t to uint64_t.
> ---
>  target-arm/cpu.h    | 1 +
>  target-arm/helper.c | 8 ++++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 88e22fb..62cf48a 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -181,6 +181,7 @@ typedef struct CPUARMState {
>          uint64_t c1_sys; /* System control register.  */
>          uint64_t c1_coproc; /* Coprocessor access register.  */
>          uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
> +        uint64_t sder; /* Secure debug enable register. */
>          uint32_t nsacr; /* Non-secure access control register. */
>          uint64_t ttbr0_el1; /* MMU translation table base 0. */
>          uint64_t ttbr1_el1; /* MMU translation table base 1. */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3c12eb3..0be19f3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2344,6 +2344,14 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
>        .access = PL3_RW, .resetvalue = 0, .writefn = scr_write,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3) },
> +    { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
> +      .access = PL3_RW, .resetvalue = 0,
> +      .fieldoffset = offsetof(CPUARMState, cp15.sder) },
> +    { .name = "SDER",
> +      .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 1,

Can you keep the ordering in reginfo fields to
 .cp [if needed], .opc0 [if needed], .opc1, .crn, .crm, .opc2
please? I know we have some existing reginfo structs which don't
follow that, but for new ones we're trying to follow the ordering
used in the v8 ARM ARM (which in turn matches the order used
in MRC/MCR instructions).

:-) I inherited the misordered definition, should have fixed when I added SDER32_EL3.  Fixed in v9.

 

> +      .access = PL3_RW, .resetvalue = 0,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.sder) },
>      { .name = "NSACR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 2,
>        .access = PL3_RW | PL1_R, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
> --
> 1.8.3.2
>

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM