Re: [Qemu-devel] [PATCH v3 07/16] target-arm: Add HCR_EL2

[Peter Maydell]

On 17 June 2014 09:45, Edgar E. Iglesias <address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>

> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 7170086..b04fb5d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2107,10 +2107,36 @@ static const ARMCPRegInfo v8_el3_no_el2_cp_reginfo[] 
> = {
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> +    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> +      .type = ARM_CP_NO_MIGRATE,
> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> +      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },

Isn't this missing the .access specifier ?

>      REGINFO_SENTINEL
>  };
>
> +static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t 
> value)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    uint64_t valid_mask = HCR_MASK;
> +
> +    if (!arm_feature(env, ARM_FEATURE_EL3)) {
> +        valid_mask &= ~HCR_HCD;
> +    }

This is inconsistent. HCD isn't the only bit that's "RES0 if
EL3 unimplemented"; TSC is as well, for instance.
(In fact the RES0 definition means you don't actually have
to mask this out unless it's more convenient to do so.)

> +
> +    /* Clear RES0 bits.  */
> +    value &= valid_mask;
> +
> +    if ((raw_read(env, ri) ^ value) & (HCR_VM | HCR_RW | HCR_PTW | HCR_DC)) {
> +        tlb_flush(CPU(cpu), 1);

Could maybe use a comment about why we need a TLB flush.

> +    }
> +    raw_write(env, ri, value);
> +}
> +
>  static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
> +    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
> +      .writefn = hcr_write },
>      { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_NO_MIGRATE,
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
> --

thanks
-- PMM