Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Aaron Lindsay <alindsay@codeaurora.org>]

Phil,

On Mar 19 00:14, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > This is a bug fix to ensure 64-bit reads of this register don't read
> > adjacent data.
> > 
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9c3b5ef..fb2f983 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -367,7 +367,7 @@ typedef struct CPUARMState {
> >          uint32_t c9_data;
> >          uint64_t c9_pmcr; /* performance monitor control register */
> >          uint64_t c9_pmcnten; /* perf monitor counter enables */
> > -        uint32_t c9_pmovsr; /* perf monitor overflow status */
> > +        uint64_t c9_pmovsr; /* perf monitor overflow status */
> 
> This doesn't look correct, since this reg is 32b.
> 
> I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:
> 
>     { .name = "PMOVSR", ...
> -     ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> +     ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>       .accessfn = pmreg_access,
>       .writefn = pmovsr_write,
>       .raw_writefn = raw_write },

Nearly all of these PMU registers are 32 bits wide, but most of them are
implemented as 64-bit registers (PMCR, PMCNTEN*, PMSELR, PMINTEN* are a
few examples I see in this patch's context). My understanding is that
AArch64 register accesses are handled as 64 bits, even if the register
itself isn't that wide (though I haven't personally verified this). See
an earlier email from Peter from v2 of this patchset:

https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03983.html

Does this still look wrong to you? If so, I'll take a more thorough look
into how these accesses work.

> >          uint32_t c9_pmuserenr; /* perf monitor user enable */

Whatever we decide should likely be done to PMUSERENR too - I think I
overlooked this one before.

> >          uint64_t c9_pmselr; /* perf monitor counter selection register */
> >          uint64_t c9_pminten; /* perf monitor interrupt enables */
> > 
> 
> Regards,
> 
> Phil.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.