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

[Edgar E. Iglesias]

On Fri, Aug 01, 2014 at 02:29:28PM +0100, Peter Maydell wrote:
> 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 ?

Good catch, thanks.

> 
> >      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.)

I've added TSC. Couldn't see any others..

> 
> > +
> > +    /* 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.

I'll add one. Actually, when thinking more about it, I'm not convinced that
we will need to flush when HCR_RW changes because the 32/64bit state is part
of the tbflags.

Cheers,
Edgar