Hi Peter,
Thanks for the review.
> >
> > From: Dongjiu Geng <gengdong...@huawei.com>
> >
> > 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 <gengdong...@huawei.com>
>
> 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
