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

[Edgar E. Iglesias]

On Mon, Aug 04, 2014 at 01:48:10PM +1000, Edgar E. Iglesias wrote:
> 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..

Hmm, I was a bit confused here. HCD is RES0 if EL3 _is_ implemented.
TSC is RES0 if EL3 is not implemented.

I will fix this up but if you prefer I can drop the zeroing aswell.

Cheers,
Edgar