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 <alind...@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.
