Re: [Qemu-devel] [RESEND PATCH] target/arm: change arch timer registers access permission

[gengdongjiu]

Hi Peter,
   Thanks for the review.

> >
> > From: Dongjiu Geng <address@hidden>
> >
> > Some generic arch timer registers are Config-RW in the EL0, which
> > means the EL0 exception level can have write permission if it is
> > appropriately configured.
> >
> > When VM access registers, it firstly checks whether they have RW
> > permission, then check whether it is appropriately configured.
> > If they are defined to Ready only in EL0, even though they have been
> 
> "read only" ?
Yes, read only.

> 
> > appropriately configured, they still do not have write permission.
> > So need to add the write permission according to ARMV8 spec when
> > define it.
> >
> > Signed-off-by: Dongjiu Geng <address@hidden>
> 
> Yes, this seems to be a bug which has been present since I added the generic 
> timer support in commit 55d284af8e31b in 2013.
> 
> I'm not sure why the bug is there -- my best guess is that I incorrectly 
> copied the permission flags from the CNTFRQ register (which really is
> RO from PL0 but RW from PL1 and above) to these other registers which should 
> be fully RW from PL0.

The current logic is show below[1], handle_sys() will check whether the system 
register have write permission in PL0, if have, then check whether
It have been configured in the high PLx by ri->accessfn()


[1]:
handle_sys()
    ------> cp_access_ok(s->current_el, ri, isread)
    ------> gen_helper_access_check_cp_reg()
           ----->ri->accessfn()

[2]:
-----------------------------------------------------------------------------------------------------------------------
static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
                       unsigned int op0, unsigned int op1, unsigned int op2,
                       unsigned int crn, unsigned int crm, unsigned int rt)
{
   
    ---------------------------------------------------------------------------
       /* Check access permissions */
    if (!cp_access_ok(s->current_el, ri, isread)) {
        unallocated_encoding(s);
        return;
    }

    if (ri->accessfn) {
        ..............................
        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn, tcg_isread);
        ..............................
        }
    -------------------------------------------
}



> 
> > diff --git a/target/arm/helper.c b/target/arm/helper.c index
> > 2607d39..3db94c6 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2665,7 +2665,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] 
> > = {
> >      /* per-timer control */
> >      { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 
> > = 1,
> >        .secure = ARM_CP_SECSTATE_NS,
> > -      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
> > +      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_RW,
> 
> The PLx_{R,W,RW} constants all imply access is also possible from the higher 
> PLs, so this can just be written ".access = PL0_RW".

Ok, I will uses your suggested method and to see whether it can be workable.

> 
> thanks
> -- PMM